[Autocomplete.Combobox] Convert to functional component #2918

genevieveluyt · 2020-04-15T04:40:12Z

WHY are these changes introduced?

The Autocomplete component causes the following warning if used with server-side rendering:

Warning: Prop `aria-controls` did not match. Server: "ComboBox2" Client: "ComboBox1"

This is a known problem: #1761

Although this PR does not directly fix that problem, converting the Combobox to a functional component and having it use the useUniqueId hook is a prerequisite for fixing the automatic ID generation in the future.

WHAT is this pull request doing?

Converts the Autocomplete Combobox component into a functional component and uses the useUniqueId hook.

How to 🎩

Not easily testable without server side rendering 🤔

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import React, {useState} from 'react';

import {Page, Autocomplete} from '../src';

export function Playground() {
  const [input, setInput] = useState('');
  const [selected, setSelected] = useState<string[]>([]);

  const textFieldMarkup = (
    <Autocomplete.TextField
      label="Textfield"
      value={input}
      onChange={setInput}
    />
  );
  const options = ['one', 'two', 'three'].map((item) => {
    return {value: item, label: item};
  });
  return (
    <Page title="Playground">
      <Autocomplete
        options={options}
        selected={selected}
        onSelect={setSelected}
        textField={textFieldMarkup}
        allowMultiple
      />
    </Page>
  );
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

ghost · 2020-04-15T04:40:16Z

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

github-actions · 2020-04-15T04:42:49Z

🟢 This pull request modifies 3 files and might impact 1 other files.

Details:

All files potentially affected (total: 1)

/src/components/Autocomplete/Autocomplete.tsx

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Autocomplete/components/ComboBox/ComboBox.tsx (total: 1)

Files potentially affected (total: 1)

/src/components/Autocomplete/Autocomplete.tsx

🧩 src/components/Autocomplete/components/ComboBox/tests/ComboBox.test.tsx (total: 0)

Files potentially affected (total: 0)

BPScott

Thanks for taking this on @genevieveluyt!

I've not ran this just yet to test behaviour, but I've spotted some simplifications that could be done

src/components/Autocomplete/components/ComboBox/ComboBox.tsx

BPScott · 2020-04-15T17:35:15Z

src/components/Autocomplete/components/ComboBox/ComboBox.tsx

+    (OptionDescriptor | ActionListItemDescriptor)[]
+  >([]);
+  const [popoverActive, setPopoverActive] = useState<boolean>(false);
+  const [popoverWasActive, setPopoverWasActive] = useState<boolean>(false);


As above, though tbh I'm not very clear on why we need to track the previous value of the popover state

If we do indeed still need this then it seems like an idea case for a usePrevious utility hook. We don't currently depend upon that @shopify/react-hooks and instead roll our own helpers, so copying that hook into our utilities folder at src/utilities/use-previous.ts (and its test at `src/utilities/use-previous.test.ts) would be the right place to put it. That pattern matches our other utility hooks like useToggle.

usePrevious should save a bunch of setPopoverActive(false); setPopoverWasActive(false); code, as we can only talk about setting popoverActive, and not worry about popoverWasActive

I removed the previous state and it didn't break any tests. I'm not sure if that logic was there for a specific case not covered by tests though 🤔

src/components/Autocomplete/components/ComboBox/ComboBox.tsx

BPScott

A few more little tidyings that became apparent since the prior simplifications

src/components/Autocomplete/components/ComboBox/ComboBox.tsx

BPScott · 2020-04-17T22:44:21Z

Just gave this a click around in storybook and seems good to me. I'd appreciate some extra eyes though as I'm not that familiar with the Comobox's behaviour. @dfmcphee / @chloerice would you be able to take some time next week to give this a review too?

BPScott · 2020-04-17T22:47:27Z

Oh and a little expectation setting: This PR alone won't solve the automatic id mismatches between server and client, but the converting it to a functional component is a prerequisite of that future fix, which will be inside useUniqueId() - either through adding functionality ourselves or by leveraging useOpaqueIdentifier()

genevieveluyt · 2020-04-20T13:45:28Z

Ah ok I misunderstood. That means I'll still need to set the ID manually for the time being?

chloerice · 2020-04-20T20:51:35Z

Hey @genevieveluyt, thanks for working on this! I'll tophat/review this by EOD Wednesday 😊

chloerice

Hey @genevieveluyt, 🎩 looks good 👍(any issues I found pre-exist these changes) Thank you for contributing ❤️!!!

You'll just need to add an entry to UNRELEASED.md under Code quality to pass the change log check

- Converted `ComboBox` to a functional component ([#2918](https://github.com/Shopify/polaris-react/pull/2918))

src/components/Autocomplete/components/ComboBox/ComboBox.tsx

Co-Authored-By: Chloe Rice <[email protected]>

ghost · 2020-04-29T17:14:06Z

🎉 Thanks for your contribution to Polaris React!

BPScott · 2020-04-29T17:39:12Z

Thanks @genevieveluyt!

Fix auto id generation for combobox

ed3916f

genevieveluyt requested a review from BPScott April 15, 2020 04:41

genevieveluyt changed the title ~~[Autocomplete] Fix automatic ID generation~~ [Autocomplete.Combobox] Convert to functional component and fix automatic ID generation Apr 15, 2020

BPScott reviewed Apr 15, 2020

View reviewed changes

genevieveluyt added 6 commits April 15, 2020 15:14

Don't use state for id

e0a2077

Use type inference for state

de1ac69

Use useToggle hook

c68668c

Cleanup syntax

9b3ac03

Remove tracking previous popover state

f2e496b

Derive selectedOption from selectedIndex

786235f

BPScott reviewed Apr 17, 2020

View reviewed changes

Some more cleanup

b3405e0

genevieveluyt changed the title ~~[Autocomplete.Combobox] Convert to functional component and fix automatic ID generation~~ [Autocomplete.Combobox] Convert to functional component Apr 20, 2020

chloerice self-requested a review April 20, 2020 20:47

chloerice approved these changes Apr 27, 2020

View reviewed changes

src/components/Autocomplete/components/ComboBox/ComboBox.tsx Show resolved Hide resolved

genevieveluyt and others added 3 commits April 27, 2020 16:59

Better spacing

c4d92fd

Co-Authored-By: Chloe Rice <[email protected]>

Add entry to changelog

74202a0

Merge branch 'master' into autocomplete-combobox-id

3975bd0

chloerice merged commit dd1a852 into master Apr 29, 2020

chloerice deleted the autocomplete-combobox-id branch April 29, 2020 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Autocomplete.Combobox] Convert to functional component #2918

[Autocomplete.Combobox] Convert to functional component #2918

genevieveluyt commented Apr 15, 2020 •

edited

Loading

ghost commented Apr 15, 2020

github-actions bot commented Apr 15, 2020 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Files potentially affected (total: 0)

BPScott left a comment

BPScott Apr 15, 2020

genevieveluyt Apr 15, 2020

BPScott left a comment

BPScott commented Apr 17, 2020

BPScott commented Apr 17, 2020 •

edited

Loading

genevieveluyt commented Apr 20, 2020

chloerice commented Apr 20, 2020

chloerice left a comment •

edited

Loading

ghost commented Apr 29, 2020

BPScott commented Apr 29, 2020

[Autocomplete.Combobox] Convert to functional component #2918

[Autocomplete.Combobox] Convert to functional component #2918

Conversation

genevieveluyt commented Apr 15, 2020 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

ghost commented Apr 15, 2020

github-actions bot commented Apr 15, 2020 • edited Loading

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Files potentially affected (total: 0)

BPScott left a comment

Choose a reason for hiding this comment

BPScott Apr 15, 2020

Choose a reason for hiding this comment

genevieveluyt Apr 15, 2020

Choose a reason for hiding this comment

BPScott left a comment

Choose a reason for hiding this comment

BPScott commented Apr 17, 2020

BPScott commented Apr 17, 2020 • edited Loading

genevieveluyt commented Apr 20, 2020

chloerice commented Apr 20, 2020

chloerice left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Apr 29, 2020

BPScott commented Apr 29, 2020

genevieveluyt commented Apr 15, 2020 •

edited

Loading

github-actions bot commented Apr 15, 2020 •

edited

Loading

BPScott commented Apr 17, 2020 •

edited

Loading

chloerice left a comment •

edited

Loading